Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release/2.8.0 #17

Merged
merged 14 commits into from
Oct 24, 2024
Merged

Release/2.8.0 #17

merged 14 commits into from
Oct 24, 2024

Conversation

jholloc
Copy link
Collaborator

@jholloc jholloc commented Nov 28, 2023

Release targetting getting UDA to build on Windows CI runner

DorisBDR and others added 5 commits December 1, 2023 08:05
retrieve the server host and port from environment to have a proper comaprison
we have added now the support on the client to allow multiple servers connections
we use signal_status to send some information to the client about on-going requests
@SimonPinches
Copy link
Collaborator

Is the build / install successful on system based on OpenSSL/1.1 and OpenSSL/3, e.g. RHEL9 ?

@stephen-dixon
Copy link
Contributor

Would anyone have an issue with reverting the API calls that were broken earlier in 2.7.0 when the new "client_flags" argument was introduced? I wanted to add a header of wrapper functions for other (accidental) breaking changes to the API (such as idamFree -> udaFree) but obviously function overloads with the same name will not work in the C layer, which applies to all the client_flags functions.

In my opinion the additional argument is redundant as the call to udaClientFlags() can be made within in each function instead of taking it as an input parameter.

This change will definitely take place in the uda/3.0.0 release, but I'd also like to include it in the 2.7.6 release and update 2.8.0 to follow this change.

For reference, most instances occur within the source/client/accAPI.h file

e.g.

LIBRARY_API void setIdamProperty(const char* property, CLIENT_FLAGS* client_flags);

The proposed new function signature would be e.g.

LIBRARY_API void setIdamProperty(const char* property);

@olivhoenen
Copy link
Collaborator

@stephen-dixon I think it is fine if this change is being added to 2.8.0.

@olivhoenen
Copy link
Collaborator

Can someone add https://github.com/deepakmaroo to this PR?

@stephen-dixon
Copy link
Contributor

Can someone add https://github.com/deepakmaroo to this PR?

Do they need to be added as a collaborator to the uda project? I don't have the privileges to do this

@adam-parker1 adam-parker1 requested a review from deepakmaroo July 24, 2024 10:05
@stephen-dixon
Copy link
Contributor

@olivhoenen @DorisBDR Apologies for the confusion, but on reflection I realise we cannot remove the client flags from the API function signatures in the way I previously described, so I won't try to change them in 2.7.6.

I will still add a header to include mappings for other name changes (such as idamFree -> udaFree) and I'll commit those changes to this branch too.

@deepakmaroo
Copy link
Collaborator

deepakmaroo commented Jul 26, 2024

Hello Team,
In a use case, I am using IMAS plugin. I have build UDA 2.8.0 and uda-plugins (IMAS) on sdcc, but when tried to access uda_cli -h localhost -p 56565 "IMAS::open(uri='imas:mdsplus?path=/work/imas/shared/imasdb/ITER/3/134174/117', mode='open')"
it ends with error:
getPluginAddress.cpp:40 >> Cannot open the target shared library libimas_plugin.so: libuda_cpp.so.0: cannot open shared object file: No such file or directory [1007 help() 0 -1 IMAS IMAS ] [handleRequest] [Protocol 10 Error (Receiving Client Block)]

It works when renamed ./lib/libuda_cpp.so to ./lib/libuda_cpp.so.0

UDA 2.7.5 and previous modules has libuda_cpp.so.0 library in ./lib directory
image

which is missing in 2.8.0.

image

In initPluginList, getPluginAddress is expecting libuda_cpp.so.0 but build and installed is libuda_cpp.so.2

building pyuda wheels for windows and merging in upstream changes from main at release 2.7.6
@DorisBDR
Copy link
Collaborator

DorisBDR commented Sep 17, 2024

i'm having two issues with 2.8.0

  1. compilation error
    c/source/plugins/bytes/bytesPlugin.cpp:175:48: error: cannot bind non-const lvalue reference of type ‘char*&’ to an rvalue of type ‘char*’
    ERROR] /mnt/ITER/abadiel/udacert/uda280/mudaidamtrunk/target/main/c/source/plugins/bytes/bytesPlugin.cpp:175:48: error: cannot bind non-const lvalue reference of type ‘char*&’ to an rvalue of type ‘char*’
    [WARN] boost::split(allowed_paths, std::getenv("UDA_BYTES_PLUGIN_ALLOWED_PATHS"), boost::is_any_of(";"));
    [WARN] ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    [WARN] In file included from /usr/include/boost/algorithm/string.hpp:23,
    [WARN] from /mnt/ITER/abadiel/udacert/uda280/mudaidamtrunk/target/main/c/source/plugins/bytes/bytesPlugin.cpp:21:
    [WARN] /usr/include/boost/algorithm/string/split.hpp:140:35: note: initializing argument 2 of ‘SequenceSequenceT& boost::algorithm::split(SequenceSequenceT&, RangeT&, PredicateT, boost::algorithm::token_compress_mode_type) [with SequenceSequenceT = std::vector<std::__cxx11::basic_string >; RangeT = char*; PredicateT = boost::algorithm::detail::is_any_ofF]’
    [WARN] inline SequenceSequenceT& split(
    [WARN] ^~~~~
    [WARN] At global scope:

  2. [WARN] At global scope:
    [ERROR] cc1plus: error: unrecognized command line option ‘-Wno-use-after-free’ [-Werror]
    [ERROR] cc1plus: error: unrecognized command line option ‘-Wno-mismatched-new-delete’ [-Werror]
    [ERROR] cc1plus: error: unrecognized command line option ‘-Wno-use-after-free’ [-Werror]
    [ERROR] cc1plus: all warnings being treated as errors

FYI
[abadiel@ccs73n-1 mudaidamtrunk]$ gcc --version
gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-4)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[abadiel@ccs73n-1 mudaidamtrunk]$

replacing the code with the following two lines solved the issue in my case
std::string envuda=std::getenv("UDA_BYTES_PLUGIN_ALLOWED_PATHS");
boost::split(allowed_paths, envuda, boost::is_any_of(";"));

@DorisBDR
Copy link
Collaborator

another issue is with the version
src/main/c/CMakeLists.txt -> git archive shows 2.7.6 i had to update to 2.8.0 (in my case i downloaded a zip of the uda)

@stephen-dixon
Copy link
Contributor

another issue is with the version src/main/c/CMakeLists.txt -> git archive shows 2.7.6 i had to update to 2.8.0 (in my case i downloaded a zip of the uda)

I think the git archive will only show 2.8.0 as the version when we merge the PR and tag a new release. Was the version 2.7.6 exactly, or was it something like 2.7.6.XXXX?

@DorisBDR
Copy link
Collaborator

another issue is with the version src/main/c/CMakeLists.txt -> git archive shows 2.7.6 i had to update to 2.8.0 (in my case i downloaded a zip of the uda)

I think the git archive will only show 2.8.0 as the version when we merge the PR and tag a new release. Was the version 2.7.6 exactly, or was it something like 2.7.6.XXXX?

it was something like 2.7.6.XXXX

@stephen-dixon
Copy link
Contributor

another issue is with the version src/main/c/CMakeLists.txt -> git archive shows 2.7.6 i had to update to 2.8.0 (in my case i downloaded a zip of the uda)

I think the git archive will only show 2.8.0 as the version when we merge the PR and tag a new release. Was the version 2.7.6 exactly, or was it something like 2.7.6.XXXX?

it was something like 2.7.6.XXXX

OK this is expected behaviour until we tag the next release

@stephen-dixon
Copy link
Contributor

For the unrecognised command-line options I think it's a case of this: (ie warnings are only raised when there are other issues during compilation)

When an unrecognized warning option is requested (e.g., -Wunknown-warning),
GCC emits a diagnostic stating that the option is not recognized.

However, if the -Wno- form is used, the behavior is slightly different:
no diagnostic is produced for -Wno-unknown-warning unless other diagnostics
are being produced.

This allows the use of new -Wno- options with old compilers, but if something
goes wrong, the compiler warns that an unrecognized option is present.

@DorisBDR
Copy link
Collaborator

i have a doubt ...in this version shall we see systemd too? where and how it is managed
thanks

@DorisBDR
Copy link
Collaborator

ok i found these 2 files c/source/etc/uda@.service.in and c/source/etc/uda.socket.in i guess it is this one

@stephen-dixon
Copy link
Contributor

ok i found these 2 files c/source/etc/uda@.service.in and c/source/etc/uda.socket.in i guess it is this one

Yes that's right. I think the syntax for deploying the service is something like

sudo cp <install_dir>/etc/uda.socket <install_dir>l/etc/uda@.service /etc/systemd/system &&
sudo systemctl start uda.socket &&
sudo systemctl enable uda.socket 

@DorisBDR
Copy link
Collaborator

ok sorry for the late. So we run our battery of tests and things are ok
we also get rid of xinetd but we had to modify the provided systemd configuration - i guess it is part of the site configuration
thanks Stephen for your help there

Copy link
Contributor

@stephen-dixon stephen-dixon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server and client tests passing at UKAEA

@stephen-dixon
Copy link
Contributor

ok sorry for the late. So we run our battery of tests and things are ok we also get rid of xinetd but we had to modify the provided systemd configuration - i guess it is part of the site configuration thanks Stephen for your help there

Great to hear that tests are passing. If you're happy to approve this pull request we'll merge it now and tag a new release. Though do let us know if there's anything in the systemd config we should change first.

@stephen-dixon
Copy link
Contributor

We'd ideally like to merge and tag this next week when Jonathan is back unless there are still any outstanding issues to address? @olivhoenen @SimonPinches @DorisBDR please could you confirm, approve, or, equally, shout if you want more time. Thanks!

@stephen-dixon
Copy link
Contributor

Is the build / install successful on system based on OpenSSL/1.1 and OpenSSL/3, e.g. RHEL9 ?

I'm fairly confident all the openSSL 1.1/3 build issues are now resolved. Just tested building on Rocky9 and can confirm that that works as expected.

@olivhoenen
Copy link
Collaborator

Hopefully last tests can checked next week when @deepakmaroo is back.

What about a note somewhere in the doc to warn about the change introduced with 2.7.6, and hinting at how to get the legacy compatible API with <client/legacy_accAPI.h> ?

@DorisBDR
Copy link
Collaborator

i have a stupid question for some reasons i don't see an approve button for this PR...

@stephen-dixon
Copy link
Contributor

stephen-dixon commented Oct 10, 2024

i have a stupid question for some reasons i don't see an approve button for this PR...

having already approved this one I don't have all the options on my screen anymore, but from memory I think there should be a button called something like "begin review" in the top right corner of the page which takes you to the files-changed tab (or you can navigate there directly) which has a "review changes" button. Under this there are a couple of options like approve with comments, approve with no comments, or request changes.

image

@deepakmaroo
Copy link
Collaborator

Hopefully last tests can checked next week when @deepakmaroo is back.

What about a note somewhere in the doc to warn about the change introduced with 2.7.6, and hinting at how to get the legacy compatible API with <client/legacy_accAPI.h> ?

Hello, @stephen-dixon and @olivhoenen,
I am on it and will complete testing in a day.

@stephen-dixon
Copy link
Contributor

Hopefully last tests can checked next week when @deepakmaroo is back.

What about a note somewhere in the doc to warn about the change introduced with 2.7.6, and hinting at how to get the legacy compatible API with <client/legacy_accAPI.h> ?

For info here are the docs updates I'm proposing #48

* initial notes added to docs for installing clients

* completing manual build instructions for the uda client for all platformsv of interest in the documentation

* removing some old build instructions from readme.md and instead referencing updated instructions in the docs

* updating readme

* adding a new page 'api_changes' to the documentation

* Fix typos in api_changes.md

* Update client_installation.md
@jholloc jholloc merged commit cb503c0 into main Oct 24, 2024
33 checks passed
@jholloc jholloc deleted the release/2.8.0 branch October 24, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants